Conversation
Co-authored-by: Greg Nazario <greg@gnazar.io>
|
Cursor Agent can help with this pull request. Just |
There was a problem hiding this comment.
Pull request overview
This PR removes the vulnerable python-ecdsa dependency from the Python BDD test suite and replaces the limited functionality needed by step definitions with a local compatibility layer backed by cryptography (P-256) and coincurve (secp256k1), mitigating the Minerva timing attack risk.
Changes:
- Added
tests/python/support/ecdsa_compat.pyimplementing a minimalSigningKey/VerifyingKeyAPI subset used by Behave steps. - Updated Secp256k1 and auth-key Behave steps to use the compatibility layer and backend availability flags.
- Replaced
ecdsain Python test requirements withcryptographyandcoincurve.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/python/support/ecdsa_compat.py |
Introduces a small API shim over coincurve/cryptography to replace python-ecdsa usage in tests. |
tests/python/steps/secp256k1_steps.py |
Switches Secp256k1 steps to import SigningKey/VerifyingKey from the compatibility layer. |
tests/python/steps/auth_key_steps.py |
Uses the compatibility layer to generate Secp keys for auth-key-related steps and handles backend availability. |
tests/python/requirements.txt |
Drops ecdsa and adds cryptography + coincurve dependencies for the tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Greg Nazario <greg@gnazar.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Greg Nazario <greg@gnazar.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Greg Nazario <greg@gnazar.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Greg Nazario <greg@gnazar.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not SECP256K1_AVAILABLE: | ||
| context.world.set_error( | ||
| ImportError("ecdsa library not available for Secp256k1") | ||
| ImportError("Secp256k1 crypto backend not available") |
There was a problem hiding this comment.
Error message inconsistency: secp256k1_steps.py uses "ecdsa library not available" while this file uses "Secp256k1 crypto backend not available" and "P-256 crypto backend not available". For consistency across the test suite, consider updating secp256k1_steps.py to use similar descriptive error messages, or use the same error message format in both files.
Remove the
python-ecdsadependency and replace its usage with a compatibility layer to mitigate the Minerva timing attack vulnerability.The
python-ecdsalibrary is vulnerable to a Minerva timing attack on the P-256 curve, which can leak the internal nonce and potentially lead to private key discovery. Since thepython-ecdsaproject considers side-channel attacks out of scope, a local fix was implemented by migrating affected test steps to safer cryptographic backends (cryptographyandcoincurve).